Repository navigation
Conversation
There was a problem hiding this comment.
Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.
An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing.
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical issues can merge integration sets, produce incorrect livetime-based rates, and break L2 bin conversion, while incompatible test assertions and invalid fixtures remain.
5 open issues
Review effort: Balanced
| # The set_ids would be [0,0,0,0,0,0,0,0,0,0,1,1,1,1,1,1,1,1,1,1] | ||
| # A set can be incomplete if a frame was dropped so we can not just look for | ||
| # complete sets [0-9]. | ||
| ids: np.ndarray = np.cumsum(np.diff(mod_10, prepend=0) < 0) |
There was a problem hiding this comment.
I added a todo for this and sent an email to the HIT team trying to get more context about this very valid issue.
| # Sum livetime for each set and scale for missing frames | ||
| livetime_sum = np.bincount(livetime_set_ids, weights=livetime.values) | ||
| livetime_sum = livetime_sum * (10 / livetime_counts) |
There was a problem hiding this comment.
This is a smart way to handle the missing frames. Nice work!
There was a problem hiding this comment.
Is there a threshold for the HIT team where the livetime_counts would be too low to apply this scaling in a meaningful way? It might be worth double checking to make sure this will work if there's a case where 9 out of 10 frames are dropped/missing in the set
| ------- | ||
| np.ndarray | ||
| Indices in mod_vals where the complete pattern [0, 1, ..., 9] starts. | ||
| Set id for each frame in the dataset, starting at 0. |
There was a problem hiding this comment.
Using set ids is a nice improvement
|
This PR grew in scope by a lot. I didnt realize to the extent how much of l1a - l2 processing relies on complete sets of 10. |
| data_i = macropixel_dataset[var].values[species_i::10, energy_i] | ||
| transformed_dataset[var].values[:, energy_i] = data_i | ||
| species_i += 1 | ||
| for var in species_variables: |
There was a problem hiding this comment.
@leowerneck, Veronica alerted me to the fact that you had just made this change to 10 min chunks so I added you as a reviewer for this code. Since the HIT team wants to process data with incomplete sets, this needed to change.
aa2bb3f to
1c7ea6c
Compare
|
After a conversation with Eric Christian we decided to table this. Missing 10 minutes worth of data (for the set that has a missing frame) is not worth the effort and people may use the data not knowing that there was a dropped frame. If anyone needs that data they can get it on their own from the l0 packet. |
Change Summary
closes #3578
Overview
HIT l1b sectored rates jobs failed on 20260606. This was due to a dropped frame. The l1a calculation correctly dropped the whole set but never handled the dropped livetime values. This then created a dimension error at l1b because we had 9 more livetime values (full set- 1 dropped frame).
Curent problem example:
Set 1: complete counts, complete livetime. Kept.
Set 2: one frame missing, so incomplete counts. Dropped (current behavior).
Set 3: complete counts, but its livetime comes from the 10 minutes when set 2 was transmitted, and one of those minutes is the lost frame leaving 9 of 10 livetime values.
To fix this, the HIT team desires the following outcome:
Set 1: Keep
Set 2: Each minute of sector data includes a separate element/energy and so, if a frame is missed, we would like to keep the other nine minutes and just drop the minute with the missed frame.
Set 3: Multiplying the livetime by 10/9 (or more accurately 10/(10 - # of missed frames) is fine (scaling the livetime).
This was a pretty large change and touches a significant amount of l1a sectored counts code to fix only one date in the release range. I would like to check with them to see if they have a dropped packets problem and this fix does need to be added or maybe if they dont expect it to happen again we can skip this.
claude helped me catch these bugs and with some of the numpy vectorization. I wish I could say I did all of it!
File changes
Testing
Fixed old tests and added new tests to check dropped frame logic